Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added validate option (function) to be able to selectively filter tokens #106

Merged
merged 2 commits into from
Jan 22, 2016
Merged

Conversation

fcarreiro
Copy link

In particular, it closes issue #77.
Example, the following options prevents linkifying strings like "someword.it is great"; more specifically, it only linkifies emails, strings with an explicit protocol and, otherwise, strings starting with "www".

var options = {
    validate: function (text, type) {
        return type !== 'url' || /^(http|ftp)s?:\/\//.test(text) || text.slice(0,3) === 'www';
    }
};

Tests are also included/updated.

@@ -32,8 +32,11 @@ function tokensToNodes(tokens, opts, doc) {

for (let i = 0; i < tokens.length; i++) {
let token = tokens[i];
let validated = options.resolve(opts.validate,
token.hasProtocol ? token.hasProtocol() : false,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would take out the hasProtocol argument, since validation isn't necessarily related to the link protocol. If someone want that behaviour, they would just check the given token string e.g., type !== 'url' || /^(http|ftp)s?:\/\//.test(text)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you mean. I just find it a pity that information obtained by the tokenizer (e.g. if there is a protocol -and which one-, the domain, the "path") is lost and unaccessible by the functions. It would be nice, in the future, to be able to access some (selected?) information about the token.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, if the tokenizer already knows how to handle protocols, it is redundant to have to rehandle them with a regex (or something like that).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that sense, I wouldn't be opposed to passing the whole token as the final argument!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how much of the token interface is already exposed to the library user; assuming that "none" or "not much", then I'd rather leave it like it is in the last commit (just removing hasProtocol) for the moment.

@nfrasser
Copy link
Owner

@fcarreiro Looks good, just a few issues

it('Works with overriden options (validate)', function () {
var options_validate = {
validate: function (hasProtocol, text, type) {
return type === 'email' || (hasProtocol || text.slice(0,3) === 'www');
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would change the first check here to type !== 'url', since there could conceivably be tokens other than email

@nfrasser
Copy link
Owner

👍

nfrasser pushed a commit that referenced this pull request Jan 22, 2016
Added validate option (function) to be able to selectively filter tokens
@nfrasser nfrasser merged commit e7ac584 into nfrasser:master Jan 22, 2016
@fcarreiro
Copy link
Author

:) do you have a planned next release date? I'm wondering whether to wait for a new release and change the version in my packages file or to already use the files in /lib/ generated from master.

@nfrasser
Copy link
Owner

@fcarreiro v2.0.0-beta.9 has been released with your updates. Huge thanks for this feature!

@nfrasser nfrasser mentioned this pull request Jan 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants